Fix #5114 - Avoid same-symbol qualifier-difference case falling into wrong cast branch#5117
Fix #5114 - Avoid same-symbol qualifier-difference case falling into wrong cast branch#5117gulugulubing wants to merge 3 commits intoldc-developers:masterfrom
Conversation
thewilsonator
left a comment
thewilsonator
left a comment
There was a problem hiding this comment.
please remove the whitespace changes
OK, is it accpetable: |
| thisptrLval = DtoAllocaDump(DtoCastClass(loc, dthis, iface->type)); | ||
| auto thisVal = DtoLoad(DtoType(thistype), thisptrLval); | ||
| DImValue dthis(thistype, thisVal); | ||
| thisptrLval = DtoAllocaDump(DtoCastClass(loc, &dthis, iface->type)); |
There was a problem hiding this comment.
looks like no change was made here, except for the dangerous-looking change from heap allocated dthis to stack allocated. Possibly use-after-scope. Revert this change?
There was a problem hiding this comment.
Does the original heap allocated dthis cause potential leak-like lifetime?
There was a problem hiding this comment.
that's for you to prove ;)
A mem leak is much more benign than a use-after-scope bug.
There was a problem hiding this comment.
In the original heap version, there are no deallocation in the caller and callee, so I think there is a mem leak.
And use-after-scope does not happen, because DtoCastClass only consumes the pointer immediately and does not store it.
At last, I am a little confused here: do you mean the original code intentionlly choose mem-leak to avoid use-after-scope?
There was a problem hiding this comment.
In classes.cpp, there is similar stack allocation:
// get class ptr
LLValue *v = DtoRVal(val);
// cast to size_t
v = gIR->ir->CreatePtrToInt(v, DtoSize_t(), "");
// cast to the final int type
DImValue im(Type::tsize_t, v);
return DtoCastInt(loc, &im, _to);So I am not sure which is best practise here.
| // True dynamic interface casts (different interface symbols) are still valid | ||
| // and are covered below. | ||
| // | ||
| // RUN: %ldc -c %s |
There was a problem hiding this comment.
nitpick: line 11 already tests compilation, so you can remove line 10
gulugulubing
changed the title
4 participants
It is related to #5114. The root cause is the const(I) -> I contract-context conversion always called DtoCastClass, then due to I.isBaseOf(I, ...) return false, the program accidently fall into the DtoDynamicCastInterface which is ObjC-only and ends in unreachable for non-ObjC.
So I added an early guard for same-symbol qualifier-difference case there.
The patch is only in tocall.cpp, while objcgen.cpp and classes.cpp are modified just because of removing trailing blank.